Fix race condition with setExperimentalFeatures #8637
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| cmdBar, | ||
| }) => { | ||
| const initialCode = `@settings(defaultLengthUnit = in, experimentalFeatures = allow) | ||
| const initialCode = `@settings(defaultLengthUnit = in) |
There was a problem hiding this comment.
This is how I discovered this in #8565
| toast.success( | ||
| `Updated file experimental features level to ${level.type}` | ||
| ) | ||
| const newAst = setExperimentalFeatures(level) |
There was a problem hiding this comment.
Changing this to act like other code mods, returning the new ast to be updated the regular way, instead of just a code execution.
| // Remove once this command isn't experimental anymore | ||
| let astWithNewSetting: Node<Program> | undefined | ||
| if (kclManager.fileSettings.experimentalFeatures?.type !== 'Allow') { | ||
| const result = await setExperimentalFeatures({ type: 'Allow' }) | ||
| if (err(result)) { | ||
| return Promise.reject(result) | ||
| const ast = setExperimentalFeatures({ type: 'Allow' }) | ||
| if (err(ast)) { | ||
| return Promise.reject(ast) | ||
| } | ||
|
|
||
| astWithNewSetting = ast |
There was a problem hiding this comment.
This was the problematic part in Hole. Now, since we return an ast like other codemods, we don't even have to run two executions here, but rather pass it directly to the actual codemod, see below
|
|
||
| export async function setExperimentalFeatures( | ||
| level: WarningLevel | ||
| ): Promise<void | Error> { | ||
| const newCode = changeExperimentalFeatures(codeManager.code, level) | ||
| if (err(newCode)) { | ||
| return new Error(`Failed to set experimental features: ${newCode.message}`) | ||
| } | ||
| codeManager.updateCodeStateEditor(newCode) | ||
| await codeManager.writeToFile() | ||
| await kclManager.executeCode() | ||
| } |
There was a problem hiding this comment.
Moved to settings.ts
| * Change the meta settings for the kcl file. | ||
| * @returns the new kcl string with the updated settings. | ||
| */ | ||
| export function changeExperimentalFeatures( |
There was a problem hiding this comment.
Not changing anything here, but making it possible to run in vitest
| describe('settings.spec.ts', () => { | ||
| describe('Testing setExperimentalFeatures', () => { | ||
| it('should add the annotation and set the flag if not present', async () => { | ||
| const instance = await loadAndInitialiseWasmInstance(WASM_PATH) |
| return newCode | ||
| } | ||
|
|
||
| const result = parse(newCode, instance) |
There was a problem hiding this comment.
I don't love how we need a wrapper function on the TS side to parse the result of setting the @settings() that we then need to test. I was trying to make the interface be the KCL source, but it seems like we're just not there yet. Might be better to parse on the Rust side of the boundary. It doesn't prevent the recast-and-parse, but it would save a trip across the wasm boundary.
As it is isn't fundamentally wrong, though.
3 participants
Closes #8636, needed for #8565
And make it vitest testable!